Skip to content

Allow initial call to .getReference() from any thread #22

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 19, 2018
Merged

Allow initial call to .getReference() from any thread #22

merged 1 commit into from
Sep 19, 2018

Conversation

schmidt-sebastian
Copy link
Contributor

This allows me to run the following in a test app without crashes:

  new Thread(new Runnable() {
            @Override
            public void run() {
                try {
                    final DatabaseReference reference = FirebaseDatabase.getInstance().getReference();
                    reference.child("foo/test").setValue("done");
                    } catch (Throwable t) {
                    System.out.println(t);
                    }
            }
        }).start();

@schmidt-sebastian
Copy link
Contributor Author

/test connected-check

@wilhuff
Copy link
Contributor

wilhuff commented Sep 17, 2018

Could you explain a bit (or link to an explanation) about why we don't have to call Looper.prepare() as we used to?

@schmidt-sebastian
Copy link
Contributor Author

Could you explain a bit (or link to an explanation) about why we don't have to call Looper.prepare() as we used to?

All the code that this PR touches was added or modified in the migration. The 16.0.1 RTDB client used com.google.android.gms.libs.punchclock.threads.TracingHandler as its thread handler, but that class is GMSCore internal. I replaced this code with Android's standard Handler interface.

In my initial work to get this to work, I ran across a wide range of issues that all boiled down to the same thing: I couldn't submit work to a Handler on a thread that hadn't be initialized with Looper.prepare(). I wrongfully assumed that this was a test-only problem and that any thread used in an Android app was already prepared.

So - with the fix here to use the main looper, I am only ever using threads that have already prepared. We already do this in other parts of the SDK, such as here:

firebase-android-sdk/firebase-database/src/main/java/com/google/firebase/database/android/AndroidPlatform.java

Line 93 in 00d4626

Handler handler = new Handler(applicationContext.getMainLooper());

Thus, I don't need the lines I added to the integration tests anymore.

That being said, I really want @bjornick to chime in on this as well.

wilhuff
wilhuff approved these changes Sep 18, 2018
View reviewed changes
Copy link
Contributor

@wilhuff wilhuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bjornick
Copy link
Contributor

Is there any concern about running those callbacks on the main thread? It almost feels like we want to run that in a separate thread of our own choosing, right?

@schmidt-sebastian
Copy link
Contributor Author

Is there any concern about running those callbacks on the main thread? It almost feels like we want to run that in a separate thread of our own choosing, right?

We used to expose this as an option (in the pre-GMSCore days), but since 3.x the callbacks only ever fire on the main thread.

@schmidt-sebastian schmidt-sebastian merged commit 7005c97 into master Sep 19, 2018
@schmidt-sebastian schmidt-sebastian mentioned this pull request Sep 20, 2018
Closed
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt-fixthreading branch October 15, 2018 17:25
@saurabhdtu saurabhdtu mentioned this pull request Nov 20, 2018
Closed
@firebase firebase locked and limited conversation to collaborators Oct 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bjornick bjornick bjornick approved these changes

@wilhuff wilhuff wilhuff approved these changes

Assignees

@bjornick bjornick

Labels
size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@schmidt-sebastian @wilhuff @bjornick @google-oss-bot